JBRes-8425: Add memory for renaming transformations#41
Conversation
Qodana Community for JVM1 new problem were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/qodana-action@v2025.1.1
with:
upload-result: trueContact Qodana teamContact us at qodana-support@jetbrains.com
|
Vladislav0Art
left a comment
There was a problem hiding this comment.
There is a major comment about the selected architecture and a better alternative: here.
...thub/pderakhshanfar/codecocoonplugin/components/transformations/MemoryAwareTransformation.kt
Outdated
Show resolved
Hide resolved
...thub/pderakhshanfar/codecocoonplugin/components/transformations/MemoryAwareTransformation.kt
Outdated
Show resolved
Hide resolved
| return emptyList() | ||
| } |
There was a problem hiding this comment.
In all places where you return emptyList in this getNameSuggestions function, you should return generateNewClassNames(psiClass).
Otherwise, this solution doesn't map all valid inputs into a successful rename (although the previous solution without any memory would).
There was a problem hiding this comment.
As discussed on Slack, since we use the memory as a way to make the transformations deterministic, returning an emptyList is the desired behavior.
...hub/pderakhshanfar/codecocoonplugin/components/transformations/RenameMethodTransformation.kt
Outdated
Show resolved
Hide resolved
| private suspend fun getNameSuggestions(method: PsiMethod, memory: RenameMemory?): List<String> { | ||
| if (useMemory) { | ||
| val signature = IntelliJAwareTransformation.withReadAction { | ||
| PsiSignatureGenerator.generateSignature(method) | ||
| } | ||
| if (signature == null) { | ||
| logger.warn("Could not generate signature for method ${IntelliJAwareTransformation.withReadAction { method.name }}") |
There was a problem hiding this comment.
Looking at these getNameSuggestions(psiComponent, memory), it's apparent that you can reduce it into a "cached-or-call-method" semantic:
[suspend] fun <R> Memory.cachedOrCall(signature: String?, callback: [suspend] () -> R): R {
val memory = this
if (signature != null && memory.has(signature)) return memory.getValue(signature)
// otherwise, when missing, execute a callback
val result = callback()
if (signature != null) memory.store(signature, result)
return result
}
// How To Use:
// inside your transformation
val methodSignature = readAction { SignatureGenerator.signatureOf(psiMethod) }
val suggestions = memory.cachedOrCall(methodSignature) {
suggestNewMethodNamesByAI(/* whatever */)
}
return suggestionsThis way, your memory can be used not only for renames, but for everything; which is needed by my PR as well.
There was a problem hiding this comment.
Since we do not want the fallback to be an LLM call, this is not necessary, but i restructured the logic in 128b06d to split the two more clearly.
src/main/kotlin/com/github/pderakhshanfar/codecocoonplugin/memory/RenameMemory.kt
Outdated
Show resolved
Hide resolved
| companion object { | ||
| private const val MEMORY_DIR = ".codecocoon-memory" | ||
|
|
||
| /** | ||
| * The directory where memory files are stored. | ||
| * Defaults to the CodeCocoon-Plugin root directory. | ||
| */ | ||
| private val memoryBaseDir: File by lazy { | ||
| // Get the codecocoon.config system property path and resolve the memory directory relative to it | ||
| val configPath = System.getProperty("codecocoon.config") | ||
| val baseDir = if (configPath != null) { | ||
| File(configPath).parentFile | ||
| } else { | ||
| // Fallback to current working directory if property not set | ||
| File(System.getProperty("user.dir")) | ||
| } | ||
| File(baseDir, MEMORY_DIR) | ||
| } |
There was a problem hiding this comment.
memoryBaseDir makes the save location very rigid.
It should be a caller's responsibility to define where this memory instance saves its data.
I'd place this folder selection logic somewhere to the upper layer of the project (HeadlessStarter or ExecutionService), and make the memoryBaseDir a constructor parameter of a memory instance.
I'd make it optionally parameterizable from the CLI: Useful for the eval pipeline to decide where to store the memory (i.e., creating and parsing this filepath somewhere in the HeadlessStarter).
There was a problem hiding this comment.
Fair, I added it to the the codecocoon.yaml config file, parsed in ConfigLoader and passed as a parameter via the MemoryAwareTransformation's config (it should be injected).
I agree the CLI parameter would be nice for eval, but none of the others are parameterized, so I skipped for now. Maybe we should consider doing it for all configs.
See 18fba0c
| /** | ||
| * Manages persistent storage of rename operations to enable deterministic transformations. | ||
| * | ||
| * Memory files are stored in the CodeCocoon-Plugin directory under `.codecocoon-memory/` | ||
| * and are organized by project name to allow tracking multiple projects independently. | ||
| */ | ||
| class RenameMemory(private val projectName: String) { | ||
|
|
||
| private val logger = thisLogger().withStdout() | ||
|
|
There was a problem hiding this comment.
❗️MAJOR API Concern about RenameMemory and MemoryAwareTransformation ❗️
This comment addresses some architectural caveats of how memory API is current integrated with transformations.
You don't need this RenameMemory class be rename-specific; in fact, it doesn't need to know what type of information it stores whatsoever.
What you have is a general-purpose interface of any persistent storage, like Redis/Database or even a simple in-memory Map<String, String>.
I suggest turning this API into an interface Memory with a single implementation that stores entries under a filepath given by the caller:
// you only need these 4 methods (maybe you don't even need `size` method)
interface Memory<K, V> {
fun get(key: K): V?
// returns the previous value if present, otherwise `null`
fun put(key: K, value: V): V?
fun dump(): Unit // or `save`
fun size(): Int/Long
}Even better approach is to make it AutoCloseable, since you always need to "save" your memory once you're done with it. Do it as:
interface Memory<K, V> : AutoCloseable {
// ... as above ...
fun close() {
// we kinda know how to close it already:
this.save() // or this.dump()
}
}Given this interface, you write an implementation that stores data on disk:
class PersistentProjectMemory(
filepath: String, // base directory where to store/from where to load the JSON file with cached entries
projectName: String,
) : Memory<String, String> {
// ...
}This gives you a high level of freedom of how to integrate this memory with transformations; you can:
- Create a single globally defined Memory instance shared by all transformations (somewhere in the
TransformationServiceorHeadlessStarter). - Create a memory instance for every transformation applied (I think this is the approach you currently have with
MemoryAwareTransformation).
Next, you can create a memory instance either project-wide or per-transformation in the TransformationService:
// inside `TransformationService`
class TransformationService {
fun applyTransformations() {
// ...
// (1): define a memory instance here -> you get a single memory for an entire project under transformation
val globalMemory = PersistentProjectMemory(baseDir, projectName)
for (tr in transformations) {
// (2): define memory instance here -> you get a per-transformation memory instance
val memory = PersistentProjectMemory(baseDir, projectName)
try {
executor.execute(tr, context, memory)
} finally {
// IMPORTANT: we successfully "close" the resource, i.e. dump data on disk/save in the db or Redis, etc.
memory.close() // aka `memory.save()/memory.dump()`
}
}
}
}As for the transformations (namely, IntelliJAwareTransformation interface), I'd simply add another parameter memory: Memory into apply method. The transformations that can benefit from memory, will do so; otherwise, it'll be a no-op parameter, which is fine for us.
Problems with MemoryAwareTransformation:
- It removes the freedom of selecting a scope for your memory (i.e., you always have it per-transformation; you cannot make it project-scoped, or place anywhere else). This is because
MemoryAwareTransformationuses inheritance, which in this case is inferior to the compositional approach. - It tightly couples transformation and persistent memory.
- Besides, we spawned quite a lot of transformation-related classes; making it even larger hinders maintainability.
In other words, the main problem with MemoryAwareTransformation is that it uses inheritance and couples memory and transformations way too strongly.
There was a problem hiding this comment.
Co-authored this with Claude. The following changes were made in 69c7d6b:
- Created generic
Memory<K,V>interface - implementation-agnostic, supportsAutoCloseable - Renamed
RenameMemory→PersistentMemory - Changed from inheritance to composition - memory now passed as parameter to apply(), not managed by base class
- Global memory scope -
TransformationServicecreates one instance per project via .use {}, automatic save on close - Deleted
MemoryAwareTransformation- no longer needed
I left an instance running to see if I didn't break anything with the refactoring. I'll report in the morning if there's anything.
src/main/kotlin/com/github/pderakhshanfar/codecocoonplugin/memory/RenameMemory.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/github/pderakhshanfar/codecocoonplugin/memory/RenameMemory.kt
Outdated
Show resolved
Hide resolved
Responsible for managing the memory file on disk.
Middleware between the transformations and `RenameMemory` . Extends `SelfManagedTransformation`.
Responsible for generating unique, deterministic signature for `PsiElement`s.
`associateWith` was calling the LLM for rename suggestions at every item in the suggestions list
4a1129d to
f49e543
Compare
Vladislav0Art
left a comment
There was a problem hiding this comment.
I approve 👍🏻, with several comments recommended to address, though.
core/src/main/kotlin/com/github/pderakhshanfar/codecocoonplugin/memory/Memory.kt
Show resolved
Hide resolved
src/main/kotlin/com/github/pderakhshanfar/codecocoonplugin/appstarter/HeadlessModeStarter.kt
Outdated
Show resolved
Hide resolved
...thub/pderakhshanfar/codecocoonplugin/components/transformations/RenameClassTransformation.kt
Outdated
Show resolved
Hide resolved
...thub/pderakhshanfar/codecocoonplugin/components/transformations/RenameClassTransformation.kt
Outdated
Show resolved
Hide resolved
...hub/pderakhshanfar/codecocoonplugin/components/transformations/RenameMethodTransformation.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/com/github/pderakhshanfar/codecocoonplugin/config/ConfigLoader.kt
Show resolved
Hide resolved
|
|
||
| // Manual Maven import trigger | ||
| if (configurator is MavenCommandLineInspectionProjectConfigurator) { | ||
| val mavenManager = MavenProjectsManager.getInstance(project) | ||
| logger.info("Triggering Maven import... (current modules: ${mavenManager.projects.size})") | ||
|
|
||
| ApplicationManager.getApplication().invokeAndWait { | ||
| mavenManager.forceUpdateAllProjectsOrFindAllAvailablePomFiles() | ||
| } | ||
|
|
||
| // Wait for Maven modules to appear - stable count for 3 seconds | ||
| var attempts = 0 | ||
| var lastCount = 0 | ||
| var stableCount = 0 | ||
|
|
||
| while (attempts < 120) { | ||
| waitForInvokeLaterActivities() | ||
| val count = mavenManager.projects.size |
There was a problem hiding this comment.
Let's move the entire logic into resolveMavenProject function or something. Why do we need it and why does it use some counts and thread sleeps?
Changes
RenameMemoryMemoryAwareTransformation:SelfManagedTransformation.RenameMemoryRenameTransformationsto use memory.Design questions
useMemoryis an individual config under the transformation. Should we make it a global flag?